-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-16181: Use incrementalAlterConfigs when updating broker configs by kafka-configs.sh #15304
base: trunk
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I left few small comments
@@ -405,7 +420,7 @@ object ConfigCommand extends Logging { | |||
val alterOptions = new AlterConfigsOptions().timeoutMs(30000).validateOnly(false) | |||
val alterLogLevelEntries = (configsToBeAdded.values.map(new AlterConfigOp(_, AlterConfigOp.OpType.SET)) | |||
++ configsToBeDeleted.map { k => new AlterConfigOp(new ConfigEntry(k, ""), AlterConfigOp.OpType.DELETE) } | |||
).asJavaCollection | |||
).asJavaCollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change maybe not needed!
core/src/test/scala/integration/kafka/admin/ConfigCommandIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/admin/ConfigCommandIntegrationTest.scala
Outdated
Show resolved
Hide resolved
@@ -349,7 +349,7 @@ object ConfigCommand extends Logging { | |||
} | |||
|
|||
@nowarn("cat=deprecation") | |||
private[admin] def alterConfig(adminClient: Admin, opts: ConfigCommandOptions): Unit = { | |||
private[admin] def alterConfig(adminClient: Admin, opts: ConfigCommandOptions, useIncrementalAlterConfigs: Boolean): Unit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't we set useIncrementalAlterConfigs
to true
as default specially as I can see that later we set it with true any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I added a case of updating broker configs with useIncrementalAlterConfigs=false
, also add a case of updating topic configs which only support incrementalAlterConfigs
.
"--entity-type", "brokers", | ||
"--entity-default") | ||
), true) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see any tests for useIncrementalAlterConfigs
set to false. Can we test this path as well if possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case doesn't make sense because we can't produce UnsupportedVersionException
by setting inter.broker.protocol.version, so I'm waiting for help from other reviewers, and just leave it unchanged for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dengziming One option would be to mock the admin client. This would allow you to verify the behavior. Otherwise, we could also run the tool against an old version in a system test.
efc3eaf
to
42aa586
Compare
115573d
to
659cb08
Compare
@dajac Hello, this PR is ready for review except the upgrade docs, PTAL. |
This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch) If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. |
More detailed description of your change
This PR implement KIP-1011, use incrementalAlterConfigs by default to alter broker configurations instead of the deprecated alterConfigs API, and fall back to deprecated alterConfigs API if the broker doesn't support incrementalAlterConfigs API.
Summary of testing strategy (including rationale)
UnsupportedVersionException
by setting inter.broker.protocol.versionCommitter Checklist (excluded from commit message)